Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix snapshot tests #129

Merged
merged 1 commit into from
Jan 20, 2025
Merged

Fix snapshot tests #129

merged 1 commit into from
Jan 20, 2025

Conversation

PabRod
Copy link
Contributor

@PabRod PabRod commented Jan 17, 2025

Fix issue #128.

Use ubuntu-22.04 instead of ubuntu-latest.
@PabRod PabRod self-assigned this Jan 17, 2025
@PabRod PabRod linked an issue Jan 17, 2025 that may be closed by this pull request
@PabRod PabRod changed the title Fix #128 Fix snapshot tests Jan 17, 2025
@PabRod PabRod mentioned this pull request Jan 17, 2025
@PabRod PabRod marked this pull request as ready for review January 17, 2025 11:05
@PabRod
Copy link
Contributor Author

PabRod commented Jan 17, 2025

Changes

Problem (kind of) solved.

The image snapshots are now only tested for ubuntu-22.04, the exact version I used for generating them. The remaining tests are run for windows-latest, macOS-latest and ubuntu-22.04.

Comments

It could be a good idea to do one of these two in the near future:

  1. Transfer the snapshot tests to the same system @mdingemanse is using (macOS?). If we get together in the same room, that shouldn't take long.
    or
  2. Abandon using snapshots. We will still be more reproducible than 90% of academic code 😅

@PabRod PabRod requested a review from mdingemanse January 17, 2025 11:05
Copy link
Contributor

@mdingemanse mdingemanse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good enough solution for now. Thank you!

@mdingemanse
Copy link
Contributor

On the larger question: Since the point of the snapshot is not pixel-perfect alignment across platforms anyway —OS difference in viewports, available fonts & other settings mean this is out of reach— I don't think we should be comparing them across OSs.

I also still don't get why this results in an error as opposed to a snapshot that we have to approve manually as "similar enough" in testthat output.

If that error cannot be avoided, snapshots are useless to us.

@PabRod
Copy link
Contributor Author

PabRod commented Jan 20, 2025

On the larger question: Since the point of the snapshot is not pixel-perfect alignment across platforms anyway —OS difference in viewports, available fonts & other settings mean this is out of reach— I don't think we should be comparing them across OSs.

I fully agree.

I also still don't get why this results in an error as opposed to a snapshot that we have to approve manually as "similar enough" in testthat output.

Computers are notoriously bad at common sense. Testing "similar enough" as opposed to "identical" would mean opening a whole new can of worms1.

If that error cannot be avoided, snapshots are useless to us.

Not sure if useless is the word I'll use, but indeed, they become quite annoying.

Footnotes

  1. A can I opened once, before snapshots existed. I wrote a suite of sleep-wake simulators for my PhD thesis, and I had to make sure my results matched the figures in the papers they were defined. I did it by translating to numerics their most salient features, such as range, approximate position of maxima and minima, etc. Example here.

@PabRod PabRod merged commit 2700536 into main Jan 20, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

failing R CMD CHECK
2 participants